MQ-1267 Change queues oldestMessageTimestamp to use kj::Date#6445
MQ-1267 Change queues oldestMessageTimestamp to use kj::Date#6445KennethRuan wants to merge 2 commits intocloudflare:mainfrom
Conversation
KennethRuan
force-pushed
the
kruan/enhance-queues-oldest-message-timestamp-type
branch
2 times, most recently
from
d73a4e7 to
1b54323
Compare
KennethRuan
force-pushed
the
kruan/enhance-queues-oldest-message-timestamp-type
branch
from
1b54323 to
b48718e
Compare
|
/bigbonk review dis |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks good. The change from number to Date | undefined is well-motivated, the sentinel-clearing approach is clean, and the parseQueueResponse template nicely consolidates the repeated parse-and-fixup pattern per npaun's earlier suggestion.
A few minor observations below, but nothing blocking.
|
Review submitted. Here's a summary of what I found: Overall: Approved. The PR cleanly changes Key observations posted as inline comments:
|
|
Merge executed via #6481 |
2 participants
Summary
In previous PRs, new metrics were added to the Queues API surface.
#6246
#6354
#6339
As pointed out in #6246, it would be clearer to return a JS Date for
oldestMessageTimestamp. This PR implements that change so thatoldestMessageTimestampin the three APIs modified above, all returnDate | undefinedinstead ofnumber.Changes
This PR includes changes to
queue.c++andqueue.hfeatures that are gated behind an experimental flag, as such they are non-breaking. We also make a correction to themetrics()method to useJSG_REQUIRE_NONNULL(see here).Sentinel Value
We wanted to surface
oldestMessageTimestampasDate | undefinedto the user. However, sinceDate(0)(JS Date at the UNIX epoch) andundefinedappear the same over the wire, we opt to treat the zero value as a sentinel and manually set it tokj::noneso that it appears asundefinedto the end user.An additional test was also added to ensure that the sentinel value is handled properly.
Testing